Context Clearing#1379
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
docs/api/ref/Channel.md
Outdated
| broadcast(context: Context): Promise<void>; | ||
| getCurrentContext(contextType?: string): Promise<Context|null>; | ||
| addContextListener(contextType: string | null, handler: ContextHandler): Promise<Listener>; | ||
| clearContext(contextType?: string): Promise<void>; |
There was a problem hiding this comment.
I'm wondering if we should require an explicit null argument (contextType: string | null) to clear all types - which would match addContectListener or stick with this (matching getCurrentContext). Either works of course.
There was a problem hiding this comment.
To me it makes sense to keep it closer to getCurrentContext, because they seem most related to me, but happy to hear any thoughts/suggestions.
There was a problem hiding this comment.
There is a historical reason for addContextListener having an explicit null argument, the contextType argument was originally optional so the handler argument could be either first or second (when a type was specified). This was NOT good in practice so we moved to an explicit null argument and deprecated the single argument override:
https://fdc3.finos.org/docs/2.0/api/ref/DesktopAgent#addcontextlistener-deprecated
We don't have that problem here, so I agree its fine as is (matching getCurrentContext).
kriswest
left a comment
There was a problem hiding this comment.
Many thanks for taking this on @kemerava.
Could you add an entry to the CHANGLOG.md file please.
I think we should keep this open until after fdc3 for the web (#1191) is merged, when we will need to add schemas for a request and response message. The same may be needed in the bridging (the fdc3.nothing ewill propagate through normal broadcast messages, but I think this will need a specific message to indicate it happened through the clearContextAPI call).
Co-authored-by: Kris West <kris.west@interop.io>
kriswest
left a comment
There was a problem hiding this comment.
LGTM - although we could do with an example of listening for cleared context somewhere.
Holding approval until after fdc3-for-web is merged as we'll then need to add schemas to support it there. Bridging schemas are also needed, those should be added before this is merged.
Many thanks for the contribution @kemerava, much appreciated.
|
@kriswest could you please take a look at the conformance tests that I added? Thanks |
kriswest
left a comment
There was a problem hiding this comment.
Thanks for adding the conformance test cases! I think we'llneed a couple f additional cases that look the effect on user channels.
| - `ACContextHistoryLast`: In step 5. **A** retrieves the _untyped_ current context of the channel via `const currentContext = await testChannel.getCurrentContext()`. Ensure that A receives only the very last broadcast context item _of any type_. | ||
| - `ACClearHistorySpecificContext`: Perform the above test, except after step 3 clear context with `await testChannel.clearContext('fdc3.instrument')`. Ensure that in step 5 nothing is received for `const instrument = await testChannel.getCurrentContext('fdc3.instrument')` and context is received successfully for `const instrument = await testChannel.getCurrentContext('fdc3.contact')` | ||
| - `ACClearHistoryAllContexts`: Perform the above test, except after step 3 clear context with `await testChannel.clearContext('fdc3.nothing')`. Ensure that in step 5 nothing is received for `const instrument = await testChannel.getCurrentContext('fdc3.instrument')` and for `const instrument = await testChannel.getCurrentContext('fdc3.contact')` | ||
| - `ACClearHistoryAllContextsSubscribedToNothing`: Perform the above test, except after step 3 clear context with `await testChannel.clearContext('fdc3.nothing')` and in step 4 call `await testChannel.addContextListener("fdc3.nothing", handler)` instead. Ensure that after clearing context, the handler is called. |
There was a problem hiding this comment.
A few thoughts on this case:
- I think you want to clear fdc3.instrument or all types (null) rather than fdc3.nothing
- As written, the nothing handler is added after calling clear, in that case it won't receive anything on an app channel. The handler needs to be in place before the clear.
- I think this needs to be 2 cases, one for clearing all types and another for a specific type that confirms the other is not affected and that the fdc3.nothing quotes the correct type.
- Finally, I think we need at least one case that checks the behavior of
userchannele- you'll work with the channel as an app channel to clear it (either retrrieving it by name, viagetUserChannels, orgetCUrrentChannel) but we should also confirm that it gets the fdc3.nothing broadcast and that it doesn't retain the cleared context.
Co-authored-by: Kris West <kris.west@interop.io>
|
/netlify |
|
@kemerava, @robmoffat and I were talking about this recently and wondering if we should rethink the fdc3.nothing use and instead go with events... We could bring WDYT? |
@kriswest @robmoffat updated based on what I understood the logic would be. Please let me know if I need to change something else or if I misunderstood your suggestion I see I still have to fix some of the build errors too |
There was a problem hiding this comment.
Pull Request Overview
Adds a new clearContext API to channels, updates schemas and reference docs to support context-cleared events, and extends conformance tests for the new behavior.
- Introduce
clearContextmethod andcontextClearedevent in Channel API and implementation - Update JSON schemas and agent protocol to include clearContext request/response
- Enhance documentation and conformance tests for context-clearing scenarios
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| website/versioned_docs/version-2.0/api/conformance/App-Channel-Tests.md | Add versioned tests for clearing specific/all contexts |
| website/static/schemas/2.2/api/agentResponse.schema.json | Register clearContextResponse in agentResponse enum |
| website/docs/api/spec.md | Document clearContext behavior and fdc3.nothing subtype field |
| website/docs/api/ref/Events.md | Define contextCleared event in TS and .NET |
| website/docs/api/ref/Channel.md | Add clearContext and addEventListener to Channel interface |
| website/docs/api/conformance/App-Channel-Tests.md | Add conformance scenarios and test IDs for context clearing |
| packages/fdc3-standard/src/api/Events.ts | Add FDC3ContextClearedEvent TypeScript interface |
| packages/fdc3-standard/src/api/Channel.ts | Implement clearContext and addEventListener in Channel API |
| packages/fdc3-schema/schemas/api/clearContextResponse.schema.json | New JSON schema for clearContextResponse |
| packages/fdc3-schema/schemas/api/clearContextRequest.schema.json | New JSON schema for clearContextRequest |
| packages/fdc3-schema/schemas/api/appRequest.schema.json | Include clearContextRequest in appRequest schema |
| packages/fdc3-schema/schemas/api/agentResponse.schema.json | Include clearContextResponse in agentResponse schema |
| packages/fdc3-agent-proxy/src/channels/DefaultChannel.ts | Proxy clearContext and event listener to agent via messaging |
| CHANGELOG.md | Note addition of clearContext API and issue resolutions |
Comments suppressed due to low confidence (3)
website/docs/api/spec.md:259
- The documentation refers to a
subTypefield, but the actual property in events and contexts is namedcontextType. Update this to match the implementation.
To raise an intent without a context, use the [`fdc3.nothing`](../context/ref/Nothing) context type. This type exists so that applications can explicitly declare that they support raising an intent without a context (when registering an `IntentHandler` or in an App Directory). This type is also used when the context is cleared for the channel. If the optional context type is provided when performing [`clearContext`](ref/Channel.md#clearcontext), that type will be recorded in the field `subType` of [`fdc3.nothing`](../context/ref/Nothing) context type.
website/docs/api/ref/Events.md:248
- In the TypeScript definition for
FDC3ContextClearedEvent, thedetailsproperty should usecontextTypeinstead oftypeto align with other language bindings and the schema.
type: string | null;
website/docs/api/conformance/App-Channel-Tests.md:77
- [nitpick] The test IDs
ACClearContext1andACClearContext2are inconsistent with the earlierACClearHistory*naming. Consider renaming them to match the existing prefix scheme.
- `ACClearContext1`: Perform above test.
website/docs/api/ref/Events.md
Outdated
| public class Fdc3ContextClearedEvent : Fdc3Event | ||
| { | ||
| public Fdc3ContextClearedEvent(string? contextType) | ||
| : base(Fdc3EventType.ContextCleared, new Fdc3ContextClearedEvent(contextType)) |
There was a problem hiding this comment.
The .NET constructor is recursively instantiating Fdc3ContextClearedEvent instead of passing a details object. It should pass a Fdc3ContextClearedEventDetails instance to the base constructor.
| : base(Fdc3EventType.ContextCleared, new Fdc3ContextClearedEvent(contextType)) | |
| : base(Fdc3EventType.ContextCleared, new Fdc3ContextClearedEventDetails(contextType)) |
| switch (type) { | ||
| case 'contextCleared': | ||
| listener = new EventListener(this.messaging, 'contextCleared', handler); | ||
| break; |
There was a problem hiding this comment.
The addEventListener implementation throws on null event type, but the API allows subscribing to all events with null. Consider handling null to register a listener for all event types.
| break; | |
| break; | |
| case null: | |
| listener = new EventListener(this.messaging, 'allEvents', handler); // Register for all events | |
| break; |
There was a problem hiding this comment.
Copilot has a point there. It looks like this is also wrong where we listener for the channelChanged event:
FDC3/packages/fdc3-agent-proxy/src/DesktopAgentProxy.ts
Lines 77 to 85 in d4a261a
Co-pilot's suggestion to fix isn't right however. Rather EventListener will probbably ned to be adjusted to be able to take multiple types:
As we need to know the specific messages it should respond to (from a set that is wider than the scope of the listener).
website/docs/api/ref/Channel.md
Outdated
| </Tabs> | ||
|
|
||
| Used to clear the specified context type if provided, otherwise, clear all context types present in the channel. The Desktop Agent MUST update its internal representation of the context in the channel and ensure that subsequent calls to [`getCurrentContext`](#getcurrentcontext) and any new joiners to that channel (through [`joinUserChannel`](DesktopAgent#joinUserChannel) or [`addContextListener`](DesktopAgent#addContextListener)) will not receive anything for either specified context type or the most recent context until new context has been broadcast to the channel. | ||
| Desktop Agents MUST also immediately notified the apps that are listening to `contextCleared` event for this channel. If a `contextType` parameter was provided, then the `contextType` field will be set to that type, otherwise, it is omitted. |
There was a problem hiding this comment.
Grammar: change 'immediately notified' to 'immediately notify' to correct the imperative form.
| Desktop Agents MUST also immediately notified the apps that are listening to `contextCleared` event for this channel. If a `contextType` parameter was provided, then the `contextType` field will be set to that type, otherwise, it is omitted. | |
| Desktop Agents MUST also immediately notify the apps that are listening to `contextCleared` event for this channel. If a `contextType` parameter was provided, then the `contextType` field will be set to that type, otherwise, it is omitted. |
kriswest
left a comment
There was a problem hiding this comment.
Really really ready to see this merged, as I'm sure is @kemerava!
There's one small change needed to handle the null argument on addEventListener. Up to you if applying the same fix to DesktopAgentProxy's addEventListener - if not we need to raise an issue and another PR for that.
I've also suggested that EventListener is updated to take multiple types to simplify things when we have more events - technically there is only one of each type at present so null handling is just handling that one relevant type.
| switch (type) { | ||
| case 'contextCleared': | ||
| listener = new EventListener(this.messaging, 'contextCleared', handler); | ||
| break; |
There was a problem hiding this comment.
Copilot has a point there. It looks like this is also wrong where we listener for the channelChanged event:
FDC3/packages/fdc3-agent-proxy/src/DesktopAgentProxy.ts
Lines 77 to 85 in d4a261a
Co-pilot's suggestion to fix isn't right however. Rather EventListener will probbably ned to be adjusted to be able to take multiple types:
As we need to know the specific messages it should respond to (from a set that is wider than the scope of the listener).
| await listener.register(); | ||
| return listener; | ||
| } | ||
|
|
There was a problem hiding this comment.
we're going to need some cucumber tests for this otherwise you'll see the coverage dip!
| // first, ensure channel state is up-to-date | ||
| const request: ClearContextRequest = { | ||
| meta: this.messaging.createMeta(), | ||
| payload: { |
There was a problem hiding this comment.
Also, there will need to be support for this event added to the BroadcastHandler in the fdc3-web-impl, plus tests of the same there.
| A context type may also be associated with multiple intents. For example, an `fdc3.instrument` could be associated with `ViewChart`, `ViewNews`, `ViewAnalysis` or other intents. | ||
|
|
||
| To raise an intent without a context, use the [`fdc3.nothing`](../context/ref/Nothing) context type. This type exists so that applications can explicitly declare that they support raising an intent without a context (when registering an `IntentHandler` or in an App Directory). | ||
| To raise an intent without a context, use the [`fdc3.nothing`](../context/ref/Nothing) context type. This type exists so that applications can explicitly declare that they support raising an intent without a context (when registering an `IntentHandler` or in an App Directory). This type is also used when the context is cleared for the channel. If the optional context type is provided when performing [`clearContext`](ref/Channel.md#clearcontext), that type will be recorded in the field `subType` of [`fdc3.nothing`](../context/ref/Nothing) context type. |
There was a problem hiding this comment.
I'm unsure if this is still current. @kriswest indicated that the approach had been changed from using fdc3.nothing to the event based workflow. Is this still accurate?
Co-authored-by: Kris West <kris.west@interop.io>
…a/FDC3 into feature/context-clearing
Co-authored-by: Hugh Troeger <troeger.hugh@gmail.com>
|
Merging to the new branch, will create another PR from that into main |
Closes #1197
Adding context cleaning
@michael-bowen-sc